-
Notifications
You must be signed in to change notification settings - Fork 87
refactor: extract core logic of router #189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the router functionality by extracting core routing logic into the new mcpm/core/router module and updating associated references throughout the codebase. Key changes include updating import paths from mcpm/router to mcpm/core/router, moving log directory retrieval to the new log manager, and renaming internal handler functions (e.g. from get_active_servers to get_target_servers) for improved clarity in core logic.
Reviewed Changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_router.py | Updated import paths and patched internal handler names. |
| src/mcpm/utils/platform.py | Removed get_log_directory now provided by the core log manager. |
| src/mcpm/utils/config.py | Removed splitor constants as part of the refactor. |
| src/mcpm/router/sse_app.py | Updated log manager import, reflecting new module organization. |
| src/mcpm/router/router.py | Extended core router functionality; replaced direct logic with calls to core methods. |
| src/mcpm/monitor/event.py | Updated splitor constants import to use new core router definitions. |
| src/mcpm/core/utils/log_manager.py | Introduced log manager with a renamed class (ServerLogManager). |
| src/mcpm/core/router/router.py | Extracted core router logic including server management and handler functions. |
| src/mcpm/commands/router.py | Updated log manager and platform imports to support changes. |
Comments suppressed due to low confidence (2)
src/mcpm/router/router.py:121
- The inner get_prompt function calls self.get_prompt which may cause confusion with the core method of the same name. Consider renaming the inner function or adding a clarifying comment to ensure the intended method is being invoked.
result = await self.get_prompt(req.params, target_servers)
src/mcpm/router/router.py:154
- [nitpick] The log message prints 'target_servers', which is a list and may be ambiguous in context. It would be clearer to log the specific server or clarify that multiple target servers are being referenced.
logger.error(f"Error calling tool {req.params.name} on server {target_servers}: {e}")
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
SummaryRefactors the router by moving its core implementation to ReviewNice modularisation—separating protocol-agnostic logic from the HTTP layer makes the codebase cleaner and easier to reuse. • Public API is preserved via Otherwise the changes look solid and tests pass. 👍 |
calmini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
User description
Preparation for
mcpm cliPR Type
Enhancement
Description
Extract core router logic into reusable module
Move client connection handling to separate module
Consolidate log management utilities
Update import paths across codebase
Changes walkthrough 📝
6 files
Update import path for log directory utilityUpdate import path for router constantsUpdate import path for log directory utilityUpdate import path for log directory utilityRemove router constants moved to core moduleRemove log directory utility moved to core4 files
Add new client connection management moduleExtract core router logic into standalone moduleMove log directory utility and rename classRefactor to inherit from core router class1 files
Update import paths for refactored modules